Skip to content

Meshing for Triangle3d primitive #12686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 8, 2024

Conversation

mweatherley
Copy link
Contributor

@mweatherley mweatherley commented Mar 24, 2024

Objective

Solution

The Meshable trait for Triangle3d directly produces a Mesh, much like that of Triangle2d. The mesh consists only of a single triangle (the triangle itself), and its vertex data consists of:

  • Vertex positions, which are the triangle's vertices themselves (i.e. the triangle provides its own coordinates in mesh space directly)
  • Normals, which are all the normal of the triangle itself
  • Indices, which are directly inferred from the vertex order (note that this is slightly different than Triangle2d which, because of its lower dimension, has an orientation which can be corrected for so that it always faces "the right way")
  • UV coordinates, which are produced as follows:
    1. The first coordinate is coincident with the ab direction of the triangle.
    2. The second coordinate maps to be perpendicular to the first in mesh space, so that the UV-mapping is skew-free.
    3. The UV-coordinates map to the smallest rectangle possible containing the triangle, given the preceding constraints.

Here is a visual demonstration; here, the ab direction of the triangle is horizontal, left to right — the point c moves, expanding the bounding rectangle of the triangle when it pushes past a or b:

Screenshot 2024-03-23 at 5 36 01 PM Screenshot 2024-03-23 at 5 38 12 PM Screenshot 2024-03-23 at 5 37 15 PM

The UV-mapping of Triangle2d has also been changed to use the same logic.


Changelog

  • Implemented Meshable for Triangle3d.
  • Changed UV-mapping of Triangle2d to match that of Triangle3d.

Migration Guide

The UV-mapping of Triangle2d has changed with this PR; the main difference is that the UVs are no longer dependent on the triangle's absolute coordinates, but instead follow translations of the triangle itself in its definition. If you depended on the old UV-coordinates for Triangle2d, then you will have to update affected areas to use the new ones which, briefly, can be described as follows:

  • The first coordinate is parallel to the line between the first two vertices of the triangle.
  • The second coordinate is orthogonal to this, pointing in the direction of the third point.

Generally speaking, this means that the first two points will have coordinates [_, 0.], while the third coordinate will be [_, 1.], with the exact values depending on the position of the third point relative to the first two. For acute triangles, the first two vertices always have UV-coordinates [0., 0.] and [1., 0.] respectively. For obtuse triangles, the third point will have coordinate [0., 1.] or [1., 1.], with the coordinate of one of the two other points shifting to maintain proportionality.

For example:

  • The default Triangle2d has UV-coordinates [0., 0.], [0., 1.], [0.5, 1.].
  • The triangle with vertices vec2(0., 0.), vec2(1., 0.), vec2(2., 1.) has UV-coordinates [0., 0.], [0.5, 0.], [1., 1.].
  • The triangle with vertices vec2(0., 0.), vec2(1., 0.), vec2(-2., 1.) has UV-coordinates [2./3., 0.], [1., 0.], [0., 1.].

Discussion

Design considerations

  1. There are a number of ways to UV-map a triangle (at least two of which are fairly natural); for instance, we could instead declare the second axis to be essentially bc so that the vertices are always [0., 0.], [0., 1.], and [1., 0.]. I chose this method instead because it is skew-free, so that the sampling from textures has only bilinear scaling. I think this is better for cases where a relatively "uniform" texture is mapped to the triangle, but it's possible that we might want to support the other thing in the future. Thankfully, we already have the capability of easily expanding to do that with Builders if the need arises. This could also allow us to provide things like barycentric subdivision.
  2. Presently, the mesh-creation code for Triangle3d is set up to never fail, even in the case that the triangle is degenerate. I have mixed feelings about this, but none of our other primitive meshes fail, so I decided to take the same approach. Maybe this is something that could be worth revisiting in the future across the board.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Mar 24, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 24, 2024
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact labels Mar 24, 2024
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely! Looks all correct to me. Really appreciate the comments on the UVs. The new UV layout seems like an unambiguous improvement too.

Do you think we could get this added to one of the primitive examples, now that it's meshed?

Comment on lines -200 to +210
let [a, b, c] = self.vertices;
let vertices_3d = self.vertices.map(|v| v.extend(0.));

let positions = vec![[a.x, a.y, 0.0], [b.x, b.y, 0.0], [c.x, c.y, 0.0]];
let positions: Vec<_> = vertices_3d.into();
let normals = vec![[0.0, 0.0, 1.0]; 3];

// The extents of the bounding box of the triangle,
// used to compute the UV coordinates of the points.
let extents = a.min(b).min(c).abs().max(a.max(b).max(c)) * Vec2::new(1.0, -1.0);
let uvs = vec![
a / extents / 2.0 + 0.5,
b / extents / 2.0 + 0.5,
c / extents / 2.0 + 0.5,
];
let uvs: Vec<_> = triangle3d::uv_coords(&Triangle3d::new(
vertices_3d[0],
vertices_3d[1],
vertices_3d[2],
))
.into();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fancy

@@ -4,6 +4,7 @@ mod cylinder;
mod plane;
mod sphere;
mod torus;
pub(crate) mod triangle3d;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this in a different format to the other module declarations?

Copy link
Contributor Author

@mweatherley mweatherley Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pub(crate) so that the Triangle2d code can see the Triangle3d UV generation code. Maybe there's a more elegant way to accomplish that, but this seemed like the simplest thing at the time.

Co-authored-by: Jakub Marcowski <[email protected]>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid to me. I'd like to see this in an example too, but I won't block on it.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 7, 2024
Copy link
Contributor

@Chubercik Chubercik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, changes made sound reasonable.

alice-i-cecile and others added 2 commits April 8, 2024 10:09
Co-authored-by: Jakub Marcowski <[email protected]>
Co-authored-by: Jakub Marcowski <[email protected]>
@alice-i-cecile
Copy link
Member

@mweatherley I couldn't immediately get the formatting right when resolving merge conflicts. Ping me when it's fixed up and I'll merge.

Copy link
Contributor

@Chubercik Chubercik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How peculiar, my previous fmt review comment was actually a copy-paste of the CI's suggestion. Weird how that wasn't the final, desirable version.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 8, 2024
Merged via the queue into bevyengine:main with commit 956604e Apr 8, 2024
@mweatherley mweatherley deleted the triangle3d-mesh branch April 8, 2024 23:16
@alice-i-cecile alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants